Skip to content

Fix #592 - desc index#636

Closed
predictor2718 wants to merge 5 commits intophpmyadmin:6.0.xfrom
predictor2718:fix-592-desc-index
Closed

Fix #592 - desc index#636
predictor2718 wants to merge 5 commits intophpmyadmin:6.0.xfrom
predictor2718:fix-592-desc-index

Conversation

@predictor2718
Copy link
Copy Markdown
Contributor

@predictor2718 predictor2718 commented Nov 24, 2025

Description

This PR fixes incorrect parsing of DESC inside index definitions in
ALTER TABLE statements.

Example:

ALTER TABLE `t`
  ADD UNIQUE KEY `idx` (`a`, `b` DESC);

This change updates AlterOperation::parse() so that ASC and DESC
are not interpreted as new statements within index definitions.

Fixes #592.

@predictor2718
Fix ALTER index parsing: DESC should not begin a new statement (#592)
54b5586 
Signed-off-by: Nicolai Ehrhardt <245527909+predictor2718@users.noreply.github.com>
@predictor2718
Add test for DESC index (#592)
bc4e5e9 
Signed-off-by: Nicolai Ehrhardt <245527909+predictor2718@users.noreply.github.com>
@predictor2718 predictor2718 mentioned this pull request Nov 24, 2025
Open
@williamdes williamdes changed the title Fix 592 desc index Fix #592 - desc index Nov 25, 2025
williamdes
williamdes reviewed Nov 25, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@williamdes williamdes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR, I am not too sure about using the in array check
Would DeSC still trigger this ?

@predictor2718
Fix too long line - phpcs linting hint (#592)
5b35ae7 
Signed-off-by: Nicolai Ehrhardt <245527909+predictor2718@users.noreply.github.com>
@predictor2718
Add tests for ASC and lowercase keyword (#592)
eac040f 
Signed-off-by: Nicolai Ehrhardt <245527909+predictor2718@users.noreply.github.com>
@predictor2718
Copy link
Copy Markdown
Contributor Author

predictor2718 commented Nov 26, 2025

Thank you for the review!

Regarding the question about whether DeSC (or other mixed-case variants) would still trigger the check:

The parser normalizes keywords during lexing.
Lexer::parseKeyword() internally calls Context::isKeyword(), which performs a strtoupper() on the token value.
Because of this, any case variation like DESC, Desc, DeSC, etc. is always mapped to the uppercase keyword DESC.

So the condition is case-insensitive and mixed-case inputs won’t cause issues.

To be safe, I also added additional tests covering:

  • ASC
  • DESC
  • lowercase desc
  • two DESC columns

All tests pass with the current implementation.

williamdes
williamdes reviewed Jan 2, 2026
View reviewed changes
@predictor2718 predictor2718 closed this by deleting the head repository Apr 2, 2026
@williamdes
Copy link
Copy Markdown
Member

williamdes commented Apr 2, 2026 •
edited
Loading

Thank you for your reply and Happy New Year!

I have linked the files and added an Asc-Desc and Desc-Asc test.

Best regards

Hi,
Sorry we may be quite slow to merge bug fixes
Even if you closed this request I will merge your work

@williamdes williamdes self-assigned this Apr 2, 2026
@williamdes williamdes modified the milestones: 4.7.4, 5.11.2 Apr 2, 2026
@predictor2718
Copy link
Copy Markdown
Contributor Author

predictor2718 commented Apr 2, 2026

@williamdes
Damn! Sorry, I thought that this had been merged already and I just deleted my fork to create another clean new fork to work at different issues.

williamdes added a commit that referenced this pull request Apr 2, 2026
@williamdes
Merge #636 - Fix #592 - desc index lint error
5b68e01 
Pull-request: #636
Fixes: #592
Signed-off-by: William Desportes <williamdes@wdes.fr>
@williamdes
Copy link
Copy Markdown
Member

williamdes commented Apr 2, 2026

@williamdes Damn! Sorry, I thought that this had been merged already and I just deleted my fork to create another clean new fork to work at different issues.

That's okay, I merged this PR as 5b68e01 into 5.11.x
Be sure to send fixes and small improvements to 5.11.x or similar stable branches

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@williamdes williamdes williamdes approved these changes

Assignees

@williamdes williamdes

Labels

None yet

Projects

None yet

Milestone

5.11.2

Development

Successfully merging this pull request may close these issues.

2 participants

@predictor2718 @williamdes